Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix socket reuse #2526

Merged
merged 1 commit into from
Oct 24, 2023
Merged

Fix socket reuse #2526

merged 1 commit into from
Oct 24, 2023

Conversation

JC-comp
Copy link
Contributor

@JC-comp JC-comp commented Oct 23, 2023 •
edited
Loading

Summary

onedrive/src/curlEngine.d

Lines 80 to 86 in 5dedd26

// https://curl.se/libcurl/c/CURLOPT_FORBID_REUSE.html
// CURLOPT_FORBID_REUSE - make connection get closed at once after use
// Ensure that we ARE NOT reusing TCP sockets connections - setting to 0 ensures that we ARE reusing connections (we did this in v2.4.xx) to ensure connections remained open and usable
// Setting this to 1 ensures that when we close the curl instance, any open sockets are closed - which we need to do when running
// multiple threads and API instances at the same time otherwise we run out of local files | sockets pretty quickly
// The libcurl default is 1 - ensure we are configuring not to reuse connections and leave unused sockets open
http.handle.set(CurlOption.forbid_reuse,1);

The use of CURLOPT_FORBID_REUSE above to handle socket exhaustion is a misuse, it does not help solve the problem at all, and will affect the performance of reusable sockets. Please check how tcp socket termination works to see why it doesn't help.

Note: Setting this option will force the socket to be closed on every request. Even if you reuse the curlEngine instance, the socket will still be closed.

Changes

  • Remove misuse of CURLOPT_FORBID_REUSE
  • Use connection header to ask server close the connection

@JC-comp JC-comp mentioned this pull request Oct 23, 2023
Closed
@abraunegg
Copy link
Owner

@JC-comp
This appears to be 100% cleaner and a much better way to handle this situation - many thanks for this work.

@abraunegg abraunegg merged commit 1d29ca0 into abraunegg:onedrive-v2.5.0-alpha-3 Oct 24, 2023
@JC-comp JC-comp deleted the curl-reuse branch October 25, 2023 04:58
@abraunegg
Copy link
Owner

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Repository owner locked and limited conversation to collaborators Nov 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JC-comp @abraunegg